-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Actually use TargetOS=AnyOS in the build-test-job. #61745
Conversation
Match the name that's used in the job title and enable us to use TargetOS to pre-filter OS-specific tests with the new Xunit test wrapper generator.
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue DetailsMatch the name that's used in the job title and enable us to use TargetOS to pre-filter OS-specific tests with the new Xunit test wrapper generator.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Jeremy for fixing this long-standing debt!
@@ -147,7 +147,7 @@ jobs: | |||
# Zip and publish managed test components | |||
- template: /eng/pipelines/common/upload-artifact-step.yml | |||
parameters: | |||
rootFolder: $(managedTestArtifactRootFolderPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - should we redefine managedTestArtifactRootFolderPath
rather than open-coding the rootFolder
so that we don't get out of sync in other yml scripts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a single-use special case of this path, so I think we want it to be different. The "osGroup" parameter to this job is still not "AnyOS" as it needs to match the libraries build so we can correctly locate the artifacts, but we want to download this artifact into the correct folder in each other build (which is all of the other use cases of $(managedTestArtifactRootFolderPath)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that ultimately I would be happier if we could express this in a cleaner way, e.g. by introducing a new parameter "logicalOsGroup" that would be AnyOS in this case and propagating it all the way down to
- name: managedTestArtifactRootFolderPath |
but I guess we can offload that to a future cleanup task and possibly present is as an up-for-grabs issue for infra volunteers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, I don't see this as blocking feedback but please consider creating the cleanup issue and adding it to the CoreCLR infrastructure backlog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually believe such a task to be very instructive for junior team members eager to ramp up on yml scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking in #61790
<GroupBuildCmd>$(GroupBuildCmd) "/p:RuntimeFlavor=$(RuntimeFlavor)"</GroupBuildCmd> | ||
<GroupBuildCmd>$(GroupBuildCmd) "/p:RuntimeVariant=$(RuntimeVariant)"</GroupBuildCmd> | ||
<GroupBuildCmd>$(GroupBuildCmd) "/p:CLRTestBuildAllTargets=$(CLRTestBuildAllTargets)"</GroupBuildCmd> | ||
<GroupBuildCmd>$(GroupBuildCmd) "/p:__TestGroupToBuild=$(__TestGroupToBuild)"</GroupBuildCmd> | ||
<GroupBuildCmd>$(GroupBuildCmd) "/p:__SkipRestorePackages=1"</GroupBuildCmd> | ||
<GroupBuildCmd>$(GroupBuildCmd) /nodeReuse:false</GroupBuildCmd> | ||
<GroupBuildCmd>$(GroupBuildCmd) /maxcpucount</GroupBuildCmd> | ||
<GroupBuildCmd>$(GroupBuildCmd) /bl:$(ArtifactsDir)/log/$(Configuration)/InnerManagedTestBuild.$(__TestGroupToBuild).binlog</GroupBuildCmd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this back, I think I lost that in my build.proj
refactoring.
- ${{ if eq(parameters.runtimeFlavor, 'mono') }}: | ||
- name: liveRuntimeBuildParams | ||
value: ${{ format('mono.corelib+libs.ref+libs.native -rc {0} -c {1} -arch {2} -ci', coalesce(parameters.liveRuntimeBuildConfig, parameters.buildConfig), parameters.liveLibrariesBuildConfig, parameters.archType) }} | ||
- name: liveRuntimeArtifactsPathArg | ||
value: ${{ format('/p:MonoOverridePath={0}/artifacts/bin/mono/{1}{2}.{3}.{4}', $(Build.SourcesDirectory), parameters.osGroup, parameters.osSubgroup, parameters.archType, coalesce(parameters.liveRuntimeBuildConfig, parameters.buildConfig)) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious and to understand, why is this needed now?
We didn't use the runtime to build the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed now because there's no alternative to specify a different path to the CoreCLR/Mono artifacts for System.Private.CoreLib without overriding the whole path (unlike Libraries, which has overrides for various different paths that we can do when we set TargetOS=AnyOS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With #61754 or some equivalent PR, we can remove this since we won't be referencing System.Private.CoreLib directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, makes sense.
@@ -448,14 +446,17 @@ | |||
<GroupBuildCmd>$(GroupBuildCmd) "/p:TargetArchitecture=$(TargetArchitecture)"</GroupBuildCmd> | |||
<GroupBuildCmd>$(GroupBuildCmd) "/p:Configuration=$(Configuration)"</GroupBuildCmd> | |||
<GroupBuildCmd>$(GroupBuildCmd) "/p:LibrariesConfiguration=$(LibrariesConfiguration)"</GroupBuildCmd> | |||
<GroupBuildCmd>$(GroupBuildCmd) "/p:LibrariesTargetOSConfigurationArchitecture=$(LibrariesTargetOSConfigurationArchitecture)"</GroupBuildCmd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually refer to this in the libraries world as BuildSettings
might make it easier to read this property, something like LibrariesBuildSettings
Timeouts seem to be happing across multiple PRs and occur in legs that don't use this job, so I'm going to merge. |
)" This reverts commit 6ff57f1.
…otnet#61745)" (dotnet#61799)" This reverts commit ca24ab0.
Match the name that's used in the job title and enable us to use TargetOS to pre-filter OS-specific tests with the new Xunit test wrapper generator.